tests: update nix dependency to 0.28 #6474

fkm3 · 2024-04-09T21:22:41Z

nix::unistd::write now requies an trait AsFd, which RawFd does not implement, so, the struct FileDescriptor field has been switched to OwnedFd. An alternative solution is to use BorrowedFd::borrow_raw, but that requires unsafe code.

`nix::unistd::write` now requies an `trait AsFd`, which `RawFd` does not implement, so, the `struct FileDescriptor` field has been switched to `OwnedFd`. An alternative solution is to use `BorrowedFd::borrow_raw`, but that requires unsafe code.

fkm3 · 2024-04-10T01:40:35Z

Not sure how these changes could cause that FreeBSD build failure

Darksonn · 2024-04-10T06:07:41Z

Perhaps we just need to enable the aio crate feature?

Darksonn · 2024-04-10T06:21:15Z

After adding the feature, there are some new compilation failures:

error[E0599]: the method `register_raw` exists for struct `Source<AioFsync>`, but its trait bounds were not satisfied
   --> tokio/tests/io_poll_aio.rs:24:20
    |
24  |             self.0.register_raw(kq, token)
    |                    ^^^^^^^^^^^^ method cannot be called on `Source<AioFsync>` due to unsatisfied trait bounds
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/mio-aio-0.8.0/src/aio.rs:80:1
    |
80  | pub struct Source<T>{inner: T}
    | -------------------- doesn't satisfy `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/nix-0.28.0/src/sys/aio.rs:429:1
    |
429 | pub struct AioFsync {
    | ------------------- doesn't satisfy `AioFsync: nix::sys::aio::Aio`
    |
    = note: the following trait bounds were not satisfied:
            `AioFsync: nix::sys::aio::Aio`
            which is required by `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
error[E0599]: the method `deregister_raw` exists for struct `Source<AioFsync>`, but its trait bounds were not satisfied
   --> tokio/tests/io_poll_aio.rs:27:20
    |
27  |             self.0.deregister_raw()
    |                    ^^^^^^^^^^^^^^ method cannot be called on `Source<AioFsync>` due to unsatisfied trait bounds
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/mio-aio-0.8.0/src/aio.rs:80:1
    |
80  | pub struct Source<T>{inner: T}
    | -------------------- doesn't satisfy `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/nix-0.28.0/src/sys/aio.rs:429:1
    |
429 | pub struct AioFsync {
    | ------------------- doesn't satisfy `AioFsync: nix::sys::aio::Aio`
    |
    = note: the following trait bounds were not satisfied:
            `AioFsync: nix::sys::aio::Aio`
            which is required by `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
error[E0599]: the method `submit` exists for struct `Pin<&mut Source<AioFsync>>`, but its trait bounds were not satisfied
   --> tokio/tests/io_poll_aio.rs:37:21
    |
37  |             match p.submit() {
    |                     ^^^^^^ method cannot be called on `Pin<&mut Source<AioFsync>>` due to unsatisfied trait bounds
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/mio-aio-0.8.0/src/aio.rs:80:1
    |
80  | pub struct Source<T>{inner: T}
    | -------------------- doesn't satisfy `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/nix-0.28.0/src/sys/aio.rs:429:1
    |
429 | pub struct AioFsync {
    | ------------------- doesn't satisfy `AioFsync: nix::sys::aio::Aio`
    |
    = note: the following trait bounds were not satisfied:
            `AioFsync: nix::sys::aio::Aio`
            which is required by `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
error[E0599]: the method `aio_return` exists for struct `Pin<&mut Source<AioFsync>>`, but its trait bounds were not satisfied
   --> tokio/tests/io_poll_aio.rs:56:36
    |
56  |                     let result = p.aio_return();
    |                                    ^^^^^^^^^^ method cannot be called on `Pin<&mut Source<AioFsync>>` due to unsatisfied trait bounds
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/mio-aio-0.8.0/src/aio.rs:80:1
    |
80  | pub struct Source<T>{inner: T}
    | -------------------- doesn't satisfy `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/nix-0.28.0/src/sys/aio.rs:429:1
    |
429 | pub struct AioFsync {
    | ------------------- doesn't satisfy `AioFsync: nix::sys::aio::Aio`
    |
    = note: the following trait bounds were not satisfied:
            `AioFsync: nix::sys::aio::Aio`
            which is required by `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
error[E0308]: mismatched types
   --> tokio/tests/io_poll_aio.rs:139:34
    |
139 |         let source = TokioSource(mio_aio::Fsync::fsync(fd, mode, 0));
    |                      ----------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `AioFsync`, found `nix::sys::aio::AioFsync`
    |                      |
    |                      arguments to this struct are incorrect
    |
    = note: `nix::sys::aio::AioFsync` and `AioFsync` have similar names, but are actually distinct types
note: `nix::sys::aio::AioFsync` is defined in crate `nix`
   --> /.cargo/registry/src/index.crates.io-6f17d22bba15001f/nix-0.27.1/src/sys/aio.rs:432:1
    |
432 | pub struct AioFsync {
    | ^^^^^^^^^^^^^^^^^^^
note: `AioFsync` is defined in crate `nix`
   --> /.cargo/registry/src/index.crates.io-6f17d22bba15001f/nix-0.28.0/src/sys/aio.rs:429:1
    |
429 | pub struct AioFsync {
    | ^^^^^^^^^^^^^^^^^^^
    = note: perhaps two different versions of crate `nix` are being used?
note: tuple struct defined here
   --> tokio/tests/io_poll_aio.rs:20:12
    |
20  |     struct TokioSource(mio_aio::Source<nix::sys::aio::AioFsync>);
    |            ^^^^^^^^^^^
error: unused import: `SourceApi`
 --> tokio/tests/io_poll_aio.rs:4:29
  |
4 | use mio_aio::{AioFsyncMode, SourceApi};
  |                             ^^^^^^^^^
  |
  = note: `-D unused-imports` implied by `-D warnings`
  = help: to override `-D warnings` add `#[allow(unused_imports)]`
Some errors have detailed explanations: E0308, E0599.
For more information about an error, try `rustc --explain E0308`.
error: could not compile `tokio` (test "io_poll_aio") due to 6 previous errors
warning: build failed, waiting for other jobs to finish...

taiki-e · 2024-04-10T06:23:51Z

We need to update nix used in mio-aio first. (it also enables aio feature of nix)

asomers · 2024-04-10T15:59:13Z

What's the motivation to update nix?

fkm3 · 2024-04-10T17:45:11Z

What's the motivation to update nix?

I want to update the version of nix used in the Android Open Source Project and we try to keep only one version of each crate, so it would be convenient to update tokio to match versions.

asomers · 2024-04-10T18:05:24Z

What's the motivation to update nix?

I want to update the version of nix used in the Android Open Source Project and we try to keep only one version of each crate, so it would be convenient to update tokio to match versions.

The major difference between nix 0.27 and 0.28 is I/O safety. I've previously broached that topic with Tokio maintainers and they weren't interested, for good reason. So I think Tokio is best sticking with nix 0.27.

Darksonn · 2024-04-10T18:28:22Z

Our MSRV is now 1.63, so I don't think I/O safety traits are an issue anymore.

But I'm not sure it matters so much for nix, since we only use it in tests.

fkm3 · 2024-04-10T18:38:12Z

I wonder if it would make sense for the tokio tests to use the libc crate directly instead. There are also some API doc examples using nix though.

asomers · 2024-04-10T18:39:25Z

The big problem with I/O safety is lifetimes. Tokio programs need to create lots of futures with the lifetime of a file. And nontrivial Tokio programs usually need those lifetimes to be 'static. No problem if files are RawFd. But AsFd files aren't 'static. So it's hard to use them with Tokio.

fkm3 · 2024-04-10T18:43:38Z

You can always just convert the RawFd to a BorrowedFd<'_> on the spot. It requires some unsafe, but it isn't really less safe than using the RawFds directly.

asomers · 2024-04-10T18:51:09Z

You can always just convert the RawFd to a BorrowedFd<'_> on the spot. It requires some unsafe, but it isn't really less safe than using the RawFds directly.

Yeah, but if you're going to do that, then what's the point of using I/O safety at all?

Darksonn · 2024-04-10T19:11:04Z

I would generally prefer to not be stuck on an old version of a dependency forever, even if it's test-only. If avoiding that requires RawFd to BorrowedFd<'_> conversions, then so be it.

asomers · 2024-04-10T19:15:22Z

I would generally prefer to not be stuck on an old version of a dependency forever, even if it's test-only. If avoiding that requires RawFd to BorrowedFd<'_> conversions, then so be it.

In that case we can do it. But as @taiki-e said, we'll have to update mio-aio first. I'm probably the only one who can do that, and i'll need to ensure that it can still work with all downstream dependencies. BUT ATM I'm hobbled up with a broken arm so it might take me a few weeks.

Darksonn · 2024-04-10T19:22:20Z

If you're willing to do the mio-aio upgrade, then I would definitely appreciate it! There's no rush from my side, and a few weeks is not a problem at all.

@fkm3 We can carry a patch in Android's vendored version of Tokio for now.

fkm3 · 2024-04-10T20:03:13Z

Yeah, but if you're going to do that, then what's the point of using I/O safety at all?

It is the same argument for most other unsafe APIs: we want people to prefer the safe technique and make it obvious they are doing something "unsafe" that can result in bugs and security vulnerabilities. Making syscalls on a closed FD can be just as bad as a use-after-free bug, the FD number might have been reused already. I don't know how relevant that is to mio-aio's usage.

@fkm3 We can carry a patch in Android's vendored version of Tokio for now.

Sounds good. Thanks both of you

Darksonn · 2024-04-10T20:14:01Z

Marking this PR as blocked until mio-aio is upgraded.

Not possible to make this change upstream yet, see tokio-rs/tokio#6474. Bug: 333427576 Test: TreeHugger Change-Id: I950e3e7efc41e0e8e23db373ff918d11a4362359

asomers · 2024-05-12T01:03:16Z

@Darksonn @fkm3 I've finished updating mio-aio and its other downstream crates. It was hard plumbing I/O safety all the way through the stack! But I got it to work. If you like the look of #6552 , then close this PR and I'll work on getting new Nix and mio-aio crates released.

Darksonn · 2024-05-12T08:30:44Z

I looked over your PR, and it looks good to me. Thanks!

tests: update nix dependency to 0.28

163949b

`nix::unistd::write` now requies an `trait AsFd`, which `RawFd` does not implement, so, the `struct FileDescriptor` field has been switched to `OwnedFd`. An alternative solution is to use `BorrowedFd::borrow_raw`, but that requires unsafe code.

Darksonn added A-tokio Area: The main tokio crate M-io Module: tokio/io M-net Module: tokio/net labels Apr 10, 2024

This comment was marked as outdated.

Sign in to view

Add aio feature to nix dependency

6b8c485

Darksonn marked this pull request as draft April 10, 2024 20:13

Darksonn added the S-blocked Status: marked as blocked ❌ on something else such as a PR or other implementation work. label Apr 10, 2024

asomers mentioned this pull request May 12, 2024

Update nix and mio-aio dev-dependencies to the latest #6552

Merged

Darksonn closed this May 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: update nix dependency to 0.28 #6474

tests: update nix dependency to 0.28 #6474

fkm3 commented Apr 9, 2024

fkm3 commented Apr 10, 2024

This comment was marked as outdated.

Darksonn commented Apr 10, 2024

Darksonn commented Apr 10, 2024

taiki-e commented Apr 10, 2024

asomers commented Apr 10, 2024

fkm3 commented Apr 10, 2024

asomers commented Apr 10, 2024

Darksonn commented Apr 10, 2024

fkm3 commented Apr 10, 2024

asomers commented Apr 10, 2024

fkm3 commented Apr 10, 2024

asomers commented Apr 10, 2024

Darksonn commented Apr 10, 2024

asomers commented Apr 10, 2024

Darksonn commented Apr 10, 2024

fkm3 commented Apr 10, 2024

Darksonn commented Apr 10, 2024

asomers commented May 12, 2024

Darksonn commented May 12, 2024

tests: update nix dependency to 0.28 #6474

tests: update nix dependency to 0.28 #6474

Conversation

fkm3 commented Apr 9, 2024

fkm3 commented Apr 10, 2024

This comment was marked as outdated.

Darksonn commented Apr 10, 2024

Darksonn commented Apr 10, 2024

taiki-e commented Apr 10, 2024

asomers commented Apr 10, 2024

fkm3 commented Apr 10, 2024

asomers commented Apr 10, 2024

Darksonn commented Apr 10, 2024

fkm3 commented Apr 10, 2024

asomers commented Apr 10, 2024

fkm3 commented Apr 10, 2024

asomers commented Apr 10, 2024

Darksonn commented Apr 10, 2024

asomers commented Apr 10, 2024

Darksonn commented Apr 10, 2024

fkm3 commented Apr 10, 2024

Darksonn commented Apr 10, 2024

asomers commented May 12, 2024

Darksonn commented May 12, 2024